Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Lava top-level API #745

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

Lava top-level API #745

wants to merge 23 commits into from

Conversation

mathisrichter
Copy link
Contributor

@mathisrichter mathisrichter commented Jul 23, 2023 •
edited
Loading

Issue Number:

Objective of pull request: Imports all APIs that are relevant to users at the top Lava package level, enabling most users to write their programs with a single import statement, for example

import lava as lv

lif1 = lv.LIF(shape=(100,))
dense = lv.Dense(weights=np.eye(100))
lif2 = lv.LIF(shape=(100,))
lif1.s_out.connect(dense.s_in)
dense.a_out.connect(lif2.a_in)

lif1.run(run_cfg=lv.Loihi2HwCfg(), condition=lv.RunSteps(100))
lif1.stop()

or

from lava import LIF, Dense, Loihi2HwCfg, RunSteps

lif1 = LIF(shape=(100,))
dense = Dense(weights=np.eye(100))
lif2 = LIF(shape=(100,))
lif1.s_out.connect(dense.s_in)
dense.a_out.connect(lif2.a_in)

lif1.run(run_cfg=Loihi2HwCfg(), condition=RunSteps(100))
lif1.stop()

The behavior is implemented in lava/src/lava/__init__.py. Since adding an __init__ file makes the top-level lava module a regular package rather than a namespace package, it requires adding a line from the older pkgutils-style namespace packages. This method is compatible with the implicit namespace packages we use elsewhere. If desired, the same can be done in other Lava libraries, creating the same shared folder structure Lava currently has, still distributed over multiple repositories. In those other libraries, we could then also add more imports.

I am creating this as a draft PR for now to start a discussion.

See also this discussion about a flat module hierarchy.

Pull request checklist

Your PR fulfills the following requirements:

  • Issue created that explains the change and why it's needed
  • Tests are part of the PR (for bug fixes / features)
  • Docs reviewed and added / updated if needed (for bug fixes / features)
  • PR conforms to Coding Conventions
  • PR applys BSD 3-clause or LGPL2.1+ Licenses to all code files
  • Lint (flakeheaven lint src/lava tests/) and (bandit -r src/lava/.) pass locally
  • Build tests (pytest) passes locally

Pull request type

Please check your PR type:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation changes
  • Other (please describe):

What is the current behavior?

  • Currently, users have to import every class individually, specifying the full module path. For the small example above, users would have to import
from lava.proc.lif.process import LIF
from lava.proc.dense.process import Dense
from lava.magma.core.run_configs import Loihi2SimCfg
from lava.magma.core.run_conditions import RunSteps

What is the new behavior?

  • With this PR, users could write programs using just a single import statement.
import lava as lv

See longer example at the top of the PR description.

Does this introduce a breaking change?

  • Yes
  • No

Supplemental information

@mathisrichter mathisrichter self-assigned this Jul 23, 2023
@mathisrichter
Copy link
Contributor Author

Apparently, this breaks unit tests for tutorials at the moment.

@weidel-p
Copy link
Contributor

I like this idea, it would make the lava user scripts much cleaner.

I would compartmentalize the structure a bit though. Could we have categories such as "neurons", "connections", "runtime", "learning", etc?

tim-shea
tim-shea approved these changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@tim-shea tim-shea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an excellent idea. I disagree with @weidel-p about compartmentalizing, I think the biggest part of the benefit of this is to flatten out the 20 or 30 most useful bits in Lava with no additional namespaces needed, but I don't feel like that's a hill I'd die on.

The tutorial tests have been super flaky for me lately, but even if they were working 100% of the time, I'd be in favor of finding a different way to test them, since they're just generally way too long and complicated to make sense in a unit test suite.

@mathisrichter
Copy link
Contributor Author

I only briefly looked into the tutorial tests but can imagine that the pkgutils-style namespace I am using could be interfering with the way the tutorial-unittests are written. I'll have a look at that later and find a fix. But I would generally also be in favor of finding a different way of testing them. Maybe in a nightly test, together with tests at scale or for demos.

PhilippPlank
PhilippPlank approved these changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@PhilippPlank PhilippPlank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test compatibility with lava-loihi.

PhilippPlank and others added 11 commits August 7, 2023 14:37
@PhilippPlank
Merge branch 'main' into lava_api
96a00f3
@PhilippPlank
Merge branch 'main' into lava_api
cd9bcb4
@mathisrichter
Possible fix for broken tutorial unit tests.
cf7906c 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Merge branch 'lava_api' of https://github.com/lava-nc/lava into lava_api
463aa82
@mathisrichter
Removed unnecessary import.
2d9707a 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Possible tutorial test fix based on importlib.
db67116 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Ignoring unused imports in init-file.
edd5821 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Work on importlib solution
d8c9769 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Fixed import errors in tutorials.
1023f81 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Unit test for lava module imports.
3dac858 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Removed imports for CLP Processes and Spiker as they are not common.
83398ef 
Signed-off-by: Mathis Richter <[email protected]>
@mathisrichter
Copy link
Contributor Author

I fixed all issues within this repository but the underlying approach seems to not support pulling additional imports in the lava modules of other repositories (e.g., lava-loihi). This point was brought up by @bamsumit in our discussion and I verified that the different module init files overwrite each other. I have not yet looked into workarounds.

@epaxon
Copy link
Contributor

epaxon commented Aug 30, 2023

Yes, I definitely like this. This is like a statement that imports everything that users might want. What I was trying to say in the meeting, though, was that not everything is compatible with Loihi 2, and so this is like an environment that is for "CPU" backend. What I was aiming for was a way to have an import statement that is particular for "Loih2" backend, and so would only import the procs and configs particularly for Loihi 2 (and Loihi 2 emulation). Or similarly there could be a statement that is only for Loihi 1 or a statement that is for some other backend. But since this is formatted as a top-level import statement, its not clear how you would extend the idea to be only for a particular backend.

@tim-shea
Copy link
Contributor

tim-shea commented Sep 5, 2023

I fixed all issues within this repository but the underlying approach seems to not support pulling additional imports in the lava modules of other repositories (e.g., lava-loihi). This point was brought up by @bamsumit in our discussion and I verified that the different module init files overwrite each other. I have not yet looked into workarounds.

Why would lava-loihi or other package modify the set of things that get imported when you do import lava? That feels very non-deterministic and weird.

If I've installed lava-dl or lava-loihi, and there are some "top-level" components that should be easy to import from those packages, let's put those things in a lava_dl.init.py or lava.dl.init.py or something like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilippPlank PhilippPlank PhilippPlank approved these changes

@tim-shea tim-shea tim-shea approved these changes

@awintel awintel Awaiting requested review from awintel

@epaxon epaxon Awaiting requested review from epaxon

@bamsumit bamsumit Awaiting requested review from bamsumit

Assignees

@mathisrichter mathisrichter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mathisrichter @weidel-p @epaxon @tim-shea @PhilippPlank